Claude/implement e2e plan yt1o c - #68
Conversation
|
Claude finished @Anthony-Bible's task in 2m 41s —— View job PR Review: E2E Load-Investigation Test Harness
Overall this is a well-structured E2E harness with clear separation of concerns and good documentation intent. A few reliability bugs and one likely functional defect need attention before this can be trusted to pass/fail correctly in CI. Critical IssuesPort mapping mismatch will break readiness check
# deploy/e2e/docker-compose.yml:70-71
ports:
- "8090:8080" # public webhook
- "8091:8081" # internal /health, /ready ← broken if agent doesn't bind :8081The readiness loop in Test pass condition validates existence, not correctness
Shell Script IssuesUnquoted # run-test.sh:64 — unsafe
for f in $(find agent-investigations -type f -name '*.json'); doUse
Inconsistent error handling versus every other script in the harness. If 500 background for i in $(seq 1 500); do
curl -s -m 2 http://target:8080/ >/dev/null 2>&1 &
doneSpawning 500 goroutines in bash will exhaust the process table on constrained CI runners (Docker-in-Docker environments often have tight PID limits). Consider batching with Security / Permissions
chmod 0777 agent-investigations || true
Docker Compose Issues
All Duplicate
Architecture / Design Issues
Alertmanager config routes are redundant ( Both Documentation InconsistencyDocs reference The component inventory table says the load generator uses Minor Notes
|
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end load-investigation test harness under deploy/e2e/ along with a long-form planning document in docs/. The harness brings up a Docker Compose stack (target VM with node_exporter, Prometheus, Alertmanager, the code-agent itself, and a stress-ng based load generator), wires Alertmanager webhooks to the agent's /alerts/prometheus endpoint, and provides scripts to scaffold, run, and tear down the test.
Changes:
- New
docs/e2e-load-investigation-test-plan.mddescribing architecture, phases, success criteria, and risks. - New
deploy/e2e/tree: target/load-gen Dockerfiles + scripts, Prometheus/Alertmanager configs, agent alert-source config ande2e-targetskill, and a workingdocker-compose.yml. setup.sh(regenerator),run-test.sh(build/start/poll for investigation JSON/teardown), andteardown.sh.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/e2e-load-investigation-test-plan.md | High-level plan; some snippets have drifted from the committed implementation. |
| deploy/e2e/docker-compose.yml | Real stack definition; uses explicit command and per-file mounts for config/skills. |
| deploy/e2e/setup.sh | Regenerates most of the tree from heredocs (but not compose/run/teardown). |
| deploy/e2e/run-test.sh | Builds, starts, waits on /ready, polls agent-investigations/ for JSON, dumps debug on timeout. |
| deploy/e2e/teardown.sh | docker compose down -v + clears agent-investigations/. |
| deploy/e2e/target/{Dockerfile,start.sh,server.sh} | Alpine + prometheus-node-exporter + naive nc HTTP server. |
| deploy/e2e/load-gen/{Dockerfile,generate-load.sh} | stress-ng CPU/mem stress + curl flood after 30 s baseline. |
| deploy/e2e/prometheus/{prometheus.yml,alert_rules.yml} | 5 s scrape, CPU>80%/mem>85%/load>2 alerts. |
| deploy/e2e/alertmanager/alertmanager.yml | Routes both severities to code-agent webhook. |
| deploy/e2e/agent-config/alert-sources.yaml | Single Prometheus webhook source on /alerts/prometheus. |
| deploy/e2e/agent-config/skills/e2e-target/SKILL.md | Skill with investigation cheatsheet for the agent. |
| deploy/e2e/.gitignore, .env.template | Ignore .env/investigations; API-key template. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| COPY <<'EOF' /app/server.sh | ||
| #!/bin/bash | ||
| # Demo HTTP service — intentionally inefficient for load testing | ||
| # Simulates a "production service" with CPU-spikey endpoints | ||
| while true; do | ||
| echo -e "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\n\r\nok $(date)" \ | ||
| | nc -l -p 8080 -q 0 | ||
| done & | ||
| EOF | ||
|
|
||
| # Startup: node_exporter + demo service | ||
| COPY <<'EOF' /app/start.sh | ||
| #!/bin/bash | ||
| /usr/bin/node_exporter \ | ||
| --web.listen-address=0.0.0.0:9100 \ | ||
| --collector.cpu \ | ||
| --collector.meminfo \ | ||
| --collector.loadavg \ | ||
| --collector.diskstats \ | ||
| --collector.filesystem \ | ||
| --collector.netdev & | ||
| /app/server.sh | ||
| EOF | ||
|
|
||
| RUN chmod +x /app/start.sh /app/server.sh | ||
| EXPOSE 9100 8080 | ||
| CMD ["/app/start.sh"] | ||
| ``` |
| allowed-tools: | ||
| - bash | ||
| - read_file | ||
| - list_files | ||
| - fetch |
| ```yaml | ||
| version: "3.9" | ||
|
|
||
| services: | ||
| target: | ||
| build: | ||
| context: ./target | ||
| dockerfile: Dockerfile | ||
| container_name: e2e-target | ||
| ports: | ||
| - "9100:9100" # node_exporter | ||
| - "8080:8080" # demo HTTP service | ||
| networks: | ||
| - e2e-net | ||
| labels: | ||
| env: "e2e-test" | ||
|
|
||
| prometheus: | ||
| image: prom/prometheus:v3.3.1 | ||
| container_name: e2e-prometheus | ||
| volumes: | ||
| - ./prometheus:/etc/prometheus | ||
| ports: | ||
| - "9090:9090" | ||
| networks: | ||
| - e2e-net | ||
| depends_on: | ||
| - target | ||
|
|
||
| alertmanager: | ||
| image: prom/alertmanager:v0.28.1 | ||
| container_name: e2e-alertmanager | ||
| volumes: | ||
| - ./alertmanager:/etc/alertmanager | ||
| ports: | ||
| - "9093:9093" | ||
| networks: | ||
| - e2e-net | ||
| depends_on: | ||
| - prometheus | ||
|
|
||
| code-agent: | ||
| build: | ||
| context: ../.. | ||
| dockerfile: Dockerfile | ||
| container_name: e2e-code-agent | ||
| env_file: | ||
| - .env | ||
| environment: | ||
| - AGENT_SERVE_ADDR=:8080 | ||
| - AGENT_AI_PROVIDER=anthropic | ||
| - AGENT_SAFETY_AUTO_APPROVE_SAFE=true | ||
| - AGENT_SAFETY_COMMAND_VALIDATION_MODE=blacklist | ||
| - AGENT_LOG_LEVEL=debug | ||
| - AGENT_LOG_FORMAT=json | ||
| volumes: | ||
| - ./agent-config:/app/config | ||
| - ./agent-investigations:/app/.agent | ||
| ports: | ||
| - "8090:8080" # webhook (avoid clash with target :8080) | ||
| - "8091:8081" # health/ready | ||
| networks: | ||
| - e2e-net | ||
| depends_on: | ||
| - alertmanager | ||
|
|
||
| load-generator: | ||
| build: | ||
| context: ./load-gen | ||
| dockerfile: Dockerfile | ||
| container_name: e2e-load-gen | ||
| networks: | ||
| - e2e-net | ||
| depends_on: | ||
| - target | ||
|
|
||
| networks: | ||
| e2e-net: | ||
| driver: bridge | ||
| ``` |
| ```bash | ||
| # Check investigation output files | ||
| ls -la deploy/e2e/agent-investigations/ | ||
|
|
||
| # Read the investigation result | ||
| cat deploy/e2e/agent-investigations/*.json | jq . | ||
|
|
||
| # Expected fields in the result: | ||
| # - investigation_id | ||
| # - status: "completed" or "escalated" | ||
| # - confidence: 0.0-1.0 | ||
| # - findings: array of RCA findings | ||
| # - actions_taken: number of tool calls | ||
| # - duration: time spent investigating | ||
| ``` |
| ### 8.1 Real VM (instead of Docker container) | ||
|
|
||
| For a more realistic test, use a real VM: | ||
| - Spin up a GCP/GCP/AWS instance |
| # Regenerates the entire deploy/e2e/ tree from heredocs. | ||
| # The repo already ships these files committed; this script is a single | ||
| # source-of-truth refresher and a drift detector — running it on a clean | ||
| # checkout should produce `git diff --stat deploy/e2e/` == empty (apart | ||
| # from .env, which is gitignored). |
| for f in $(find agent-investigations -type f -name '*.json'); do | ||
| echo "--- $f ---" | ||
| python3 -m json.tool < "$f" 2>/dev/null || cat "$f" | ||
| echo | ||
| done |
|
@copilot apply changes based on the comments in this thread |
Applied the review-thread fixes in commit b3e9c26. This updates the E2E plan snippets to match the committed compose/target implementations, corrects allowed-tools formatting and investigation file paths, fixes the GCP/AWS typo, weakens the setup.sh drift-detector claim, and hardens run-test.sh JSON file iteration against whitespace/special chars. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@claude please review the PR now. |
|
Claude finished @Anthony-Bible's task in 2m 44s —— View job PR Review: E2E Load-Investigation Test Harness
This PR adds a well-structured E2E harness that exercises the full alert-investigation pipeline under synthetic load. The architecture is sound — the two-port design, recursive Correction to the previous Claude reviewThe previous review flagged Critical: Test pass condition doesn't validate investigation content
# after confirming FILES > 0, add before declaring PASSED:
if ! python3 -c "
import json, sys, glob
files = glob.glob('agent-investigations/**/*.json', recursive=True)
data = json.load(open(files[0]))
rca = data.get('rca_findings') or data.get('findings') or data.get('rca')
if not rca:
sys.exit(1)
" 2>/dev/null; then
echo "ERROR: investigation file exists but RCA findings are empty" >&2
exit 1
fiShell Script Issues
All other scripts in the harness use 500 background Spawning 500 shell background processes simultaneously risks exhausting the PID table on constrained CI runners. Consider batching with
Security / Permissions
The mkdir -p agent-investigations/investigations
chmod 0755 agent-investigations agent-investigations/investigationsDocker Compose IssuesDuplicate
Design IssuesAlertmanager sub-routes are redundant ( Both
Minor Notes
|
|
Claude finished @Anthony-Bible's task in 2m 12s —— View job PR Review: E2E Load-Investigation Test Harness (Post-Fix Review)
Good progress — most of the previously flagged issues have been addressed. Here's what was fixed and what still needs attention. What Was Fixed ✅
Still Needs Attention
This is still present. Since mkdir -p agent-investigations/investigations
chmod 0755 agent-investigations agent-investigations/investigationsDocs component inventory still references Line 17 (ASCII diagram) and line 42 (component table) still show Pass condition accepts The current check passes the test if an investigation has if data.get('status') not in ('completed', 'escalated'):
sys.exit(1)
if not data.get('findings') and not data.get('escalated'):
sys.exit(1)This is a judgment call — if a high-severity escalation without findings is an acceptable outcome for this test, keep as-is. Minor Notes
Prometheus still starts as soon as Alertmanager's container starts (not when it's ready), and Alertmanager starts as soon as Prometheus starts. For a short-lived E2E test this is unlikely to cause real failures since all services initialize quickly, and the
For a 300s test window, alerts can re-fire up to 4 times. This could trigger duplicate investigation runs. Acceptable for now but worth monitoring if investigation logs show duplicate entries. Overall this is in good shape. The two substantive fixes remaining are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| mkdir -p agent-investigations | ||
| # Ensure the non-root container user (UID 10001) can write here. | ||
| chmod 0777 agent-investigations || true |
There was a problem hiding this comment.
This is still 0777. Since the bind-mount replaces /app/.agent, UID 10001 needs to create the investigations/ subdirectory inside it. Pre-creating the subdirectory lets you drop to 0755:
| chmod 0777 agent-investigations || true | |
| mkdir -p agent-investigations/investigations | |
| chmod 0755 agent-investigations agent-investigations/investigations |
With the investigations/ directory pre-existing, the container user only needs write-into permission, not create-directory permission, so 0755 is sufficient.
There was a problem hiding this comment.
Pre-creating the subdirectory doesn't change the underlying permission gap. On a Linux host, writing a new file into a directory requires the write bit on that directory for the writer's UID — not just the execute bit. With 0755, only the owner has write; "others" (the container's UID 10001, which doesn't match the host owner) have read+execute but not write.
Quick proof on the runner this PR's CI uses:
$ mkdir -p /tmp/permtest/investigations && chmod 0755 /tmp/permtest/investigations
$ ls -ld /tmp/permtest/investigations
drwxr-xr-x 2 root root 4096 May 16 15:46 /tmp/permtest/investigations
$ # mode=0o755, others-writable=False — a non-root UID inside the container cannot create files here.
The Dockerfile already pre-creates /app/.agent/investigations chowned to agent:agent (UID 10001), but the bind mount ./agent-investigations:/app/.agent shadows that chowned directory with one owned by whichever UID ran mkdir on the host (root in CI). So unless we either (a) chown 10001:10001 agent-investigations on the host (needs root), or (b) add user: "10001:10001" to the code-agent service in compose to match the host owner, the only thing keeping writes working today is the 0777. Happy to switch to the user: directive approach if you'd prefer a tighter mode — that's a real fix; 0755 alone is not.
Generated by Claude Code
| data = json.load(fh) | ||
| except (OSError, json.JSONDecodeError): | ||
| sys.exit(1) | ||
| if data.get('status') not in ('completed', 'failed', 'escalated'): |
There was a problem hiding this comment.
A failed investigation that also has escalated set will pass the test here. If the intent is to validate that the agent produced a genuine root-cause analysis, consider restricting to completed and escalated only:
| if data.get('status') not in ('completed', 'failed', 'escalated'): | |
| if data.get('status') not in ('completed', 'escalated'): |
|
Claude finished @Anthony-Bible's task in 1m 54s —— View job PR Review: E2E Load-Investigation Test Harness (Latest State)
What's Fixed in the Latest Commits ✅
The harness is now significantly more reliable and correct. Two issues remain. Remaining:
|
PR Review: E2E Load-Investigation Test Harness (Post-Sync Review)
|
75bd97b to
68004ba
Compare
|
Claude finished @Anthony-Bible's task in 2m 2s —— View job PR Review: E2E Load-Investigation Test Harness (Post-Sync Review)
This sync brought one important new change — the persistence fix in New Changes in This Sync ✅
The key fix for E2E correctness. Previously Also improved: the error from
Used by Previously Fixed Issues ✅
Still Needs Attention
As discussed in thread, Anthony's counter-argument is correct: # docker-compose.yml, code-agent service
code-agent:
...
user: "${UID:-0}:${GID:-0}"With this, the container runs as the same UID that owns the bind-mount directory, and mkdir -p agent-investigations/investigations
chmod 0755 agent-investigations agent-investigations/investigationsIf the security posture is acceptable for a test-only environment and simplicity is preferred, add a comment at Docs component table still references The ASCII diagram (line 17) shows Pre-existing Bug (Not in Scope, Worth Noting)
_, cancel := context.WithCancel(ctx) // derived ctx is discardedThe derived (cancellable) context is never passed to Minor Notes
Overall the harness is in solid shape. The core persistence bug fix is the key correctness change in this sync. Two items remain: the |
…tence Adds an end-to-end test rig under deploy/e2e that brings up node_exporter, Prometheus, Alertmanager, a stress workload, and the code-agent webhook server, then asserts that a synthetic CPU/memory/load spike makes it through alerting and produces a populated RCA artifact. docs/e2e-load-investigation-test-plan.md documents the design. Also fixes a real bug uncovered while bringing the harness up: RunInvestigation persisted async completions with a status-only stub, wiping findings, completed_at, duration, confidence, and escalation state that the runner had already populated. Builds the update record from the full InvestigationResult so async investigations end up with the same persisted detail as synchronous ones, and surfaces the error instead of discarding it. Harness specifics: - HighLoadAverage/HighMemoryUtilization at severity:critical so they survive the agent's AutoInvestigateWarning=false filter. - load-gen uses --cpu 0 (all cores) and 4x1G --vm-keep to actually breach CPU/memory thresholds during the 120s stress window. - AGENT_MODEL pinned to claude-haiku-4-5-20251001 in compose; the bundled default is not an Anthropic model. - jq added to the runtime image so investigations can be inspected from inside the container. - Success criterion is "at least one investigation completed with findings"; one good RCA proves the pipeline works even if other attempts exhaust the action budget. - On timeout, dump per-file investigation summaries so failures are diagnosable without manually walking the volume. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
68004ba to
f8ee9c3
Compare
|
Claude finished @Anthony-Bible's task in 2m 21s —— View job PR Review: E2E Load-Investigation Test Harness (Latest State)
This is in solid shape. The critical persistence bug fix in What's Fixed ✅
Still Needs Attention
The fix agreed upon in thread — adding If the # docker-compose.yml, code-agent service
user: "${UID:-0}:${GID:-0}"# run-test.sh, replacing lines 23-25
export UID GID
mkdir -p agent-investigations/investigations
chmod 0755 agent-investigations agent-investigations/investigationsIf you prefer to keep Docs component table still references Line 17 (ASCII diagram): This was flagged in three consecutive previous reviews and is the only docs accuracy issue remaining. New Observation:
|
No description provided.